Skip to content

Suspending inside a constructor outside of strict mode#13200

Merged
acdlite merged 2 commits intofacebook:masterfrom
acdlite:suspendinsideconstructorinloosemode
Jul 13, 2018
Merged

Suspending inside a constructor outside of strict mode#13200
acdlite merged 2 commits intofacebook:masterfrom
acdlite:suspendinsideconstructorinloosemode

Conversation

@acdlite
Copy link
Copy Markdown
Collaborator

@acdlite acdlite commented Jul 12, 2018

Outside of strict mode, suspended components commit in an incomplete state, then are synchronously deleted in a subsequent commit. If a component suspends inside the constructor, it mounts without an instance.

This breaks at least one invariant: during deletion, we assume that every mounted component has an instance, and check the instance for the existence of componentWillUnmount.

Rather than add a redundant check to the deletion of every class component, components that suspend inside their constructor and outside of strict mode are turned into empty functional components before they are mounted. This is a bit weird, but it's an edge case, and the empty component will be synchronously unmounted regardless.

Outside of strict mode, suspended components commit in an incomplete
state, then are synchronously deleted in a subsequent commit. If a
component suspends inside the constructor, it mounts without
an instance.

This breaks at least one invariant: during deletion, we assume that
every mounted component has an instance, and check the instance for
the existence of `componentWillUnmount`.

Rather than add a redundant check to the deletion of every class
component, components that suspend inside their constructor and outside
of strict mode are turned into empty functional components before they
are mounted. This is a bit weird, but it's an edge case, and the empty
component will be synchronously unmounted regardless.
@pull-bot
Copy link
Copy Markdown

pull-bot commented Jul 12, 2018

Details of bundled changes.

Comparing: 58f3b29...5714bb9

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.2% +0.2% 638.56 KB 639.52 KB 149.62 KB 149.87 KB UMD_DEV
react-dom.development.js +0.2% +0.2% 634.69 KB 635.65 KB 148.44 KB 148.69 KB NODE_DEV
react-dom-test-utils.development.js +0.1% +0.2% 44.99 KB 45.03 KB 12.24 KB 12.27 KB UMD_DEV
react-dom-test-utils.development.js +0.1% +0.2% 44.71 KB 44.75 KB 12.18 KB 12.2 KB NODE_DEV
ReactDOM-dev.js +0.1% +0.2% 641.03 KB 641.99 KB 146.78 KB 147.02 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.1% 🔺+0.1% 276.3 KB 276.65 KB 51.67 KB 51.71 KB FB_WWW_PROD
ReactTestUtils-dev.js +0.1% +0.2% 41.89 KB 41.93 KB 11.34 KB 11.37 KB FB_WWW_DEV
ReactDOM-profiling.js +0.1% +0.1% 279.3 KB 279.65 KB 52.33 KB 52.37 KB FB_WWW_PROFILING

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.2% +0.3% 423.33 KB 424.29 KB 95.41 KB 95.67 KB UMD_DEV
react-art.development.js +0.3% +0.3% 355.88 KB 356.84 KB 78.34 KB 78.6 KB NODE_DEV
ReactART-dev.js +0.3% +0.3% 345.07 KB 346.03 KB 73.14 KB 73.38 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.2% 🔺+0.2% 147.23 KB 147.58 KB 24.92 KB 24.95 KB FB_WWW_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.3% +0.3% 360.2 KB 361.16 KB 78.86 KB 79.11 KB UMD_DEV
react-test-renderer.development.js +0.3% +0.3% 356.32 KB 357.28 KB 77.9 KB 78.15 KB NODE_DEV
ReactTestRenderer-dev.js +0.3% +0.3% 360.37 KB 361.33 KB 76.77 KB 77.01 KB FB_WWW_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.3% +0.3% 344.26 KB 345.22 KB 74.25 KB 74.5 KB NODE_DEV
react-reconciler-persistent.development.js +0.3% +0.3% 342.88 KB 343.84 KB 73.69 KB 73.94 KB NODE_DEV
react-reconciler-reflection.development.js +0.3% +0.5% 14.35 KB 14.39 KB 4.5 KB 4.52 KB NODE_DEV

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.2% +0.2% 478.6 KB 479.56 KB 105.82 KB 106.07 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+0.2% 🔺+0.1% 212.27 KB 212.62 KB 36.91 KB 36.95 KB RN_FB_PROD
ReactNativeRenderer-dev.js +0.2% +0.2% 478.34 KB 479.29 KB 105.76 KB 106.02 KB RN_OSS_DEV
ReactFabric-dev.js +0.2% +0.2% 468.83 KB 469.79 KB 103.39 KB 103.64 KB RN_FB_DEV
ReactFabric-dev.js +0.2% +0.2% 468.86 KB 469.82 KB 103.41 KB 103.66 KB RN_OSS_DEV
ReactNativeRenderer-profiling.js +0.2% +0.1% 215.35 KB 215.7 KB 37.6 KB 37.64 KB RN_FB_PROFILING

Generated by 🚫 dangerJS

Copy link
Copy Markdown
Contributor

@cyan33 cyan33 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a bit hacky but glad it fixes the issue in non-strict mode.


if (
sourceFiber.tag === ClassComponent &&
workInProgress.stateNode === null
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use current === null because we don't want to fire componentWillUnmount unless we fired componentDidMount.

In non-strict mode, suspended components commit, but their lifecycles
should not fire.
@acdlite acdlite force-pushed the suspendinsideconstructorinloosemode branch from 817cd42 to 5714bb9 Compare July 13, 2018 17:59
@acdlite acdlite merged commit 43ffae2 into facebook:master Jul 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants